Skip to content

[v1.x] fix: reject trailing newline in tool-name validation#3086

Open
maxisbey wants to merge 2 commits into
v1.xfrom
fix/tool-name-trailing-newline-v1x
Open

[v1.x] fix: reject trailing newline in tool-name validation#3086
maxisbey wants to merge 2 commits into
v1.xfrom
fix/tool-name-trailing-newline-v1x

Conversation

@maxisbey

@maxisbey maxisbey commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Backport of #3076 to v1.x.

TOOL_NAME_REGEX is $-anchored and checked with re.match; in Python, $ also matches just before a single trailing newline, so a tool name like "name\n" validated cleanly with no SEP-986 warning. Switch the check to re.fullmatch, mirroring the fix merged to main.

Also applies the same fix to the v1.x-specific instance in ResourceTemplate.matches(): a URI with a trailing newline after a literal template segment matched as if clean. (Parameter segments matching raw newlines into extracted values is a separate pre-existing looseness that also exists on main, left unchanged here.)

Credit to @Otis0408 for finding and fixing the original issue (#3084, #3076).

AI Disclaimer

With re.match, a $-anchored pattern also matches just before a single
trailing newline, so tool-name validation accepted "name\n" without a
SEP-986 warning. Switch to re.fullmatch. Backport of #3076.

Co-authored-by: otiscuilei <otiscui@icloud.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — minimal, correct backport of the re.match → re.fullmatch fix with regression tests; the one inline comment is a pre-existing issue in an untouched file and doesn't block this PR.

Extended reasoning...

Overview

This PR backports #3076 to v1.x: a one-line change in src/mcp/shared/tool_name_validation.py switching TOOL_NAME_REGEX.match(name) to TOOL_NAME_REGEX.fullmatch(name), plus two parametrized regression test cases. It fixes the classic Python pitfall where $ with re.match accepts a single trailing newline, so "name\n" previously validated cleanly with no SEP-986 warning. I verified the semantics: with fullmatch, a match ending before a trailing \n no longer spans the full string, so such names are now correctly rejected. The "a"*127 + "\n" test case is well chosen — at length 128 it passes the length check and exercises the regex path directly.

Security risks

None introduced. The change strictly tightens validation (rejects strings previously accepted), and this validation only produces warnings/rejection flags for tool registration — it is not an auth or injection boundary. No new attack surface.

Level of scrutiny

Low. This is a mechanical, well-understood one-line fix mirroring a change already merged to main, in a self-contained validation utility with dedicated test coverage. The note in the PR description that only the tool-name half of #3076 applies to v1.x is accurate for the files this PR targets; the inline comment notes a separate pre-existing instance of the same bug class in ResourceTemplate.matches() (src/mcp/server/fastmcp/resources/templates.py:89), which is worth a follow-up but is not touched by — and should not block — this PR.

Other factors

The new tests directly regression-test the fixed behavior, and the existing test suite for this module is comprehensive. The change is identical in shape to the fix on main, so there is no design decision requiring human judgment.

Comment thread src/mcp/shared/tool_name_validation.py
The same $-with-re.match pattern: a URI with a single trailing newline
after a literal template segment matched as if clean. Switch to
re.fullmatch. Parameter segments matching raw newlines into values is a
separate pre-existing looseness, unchanged here.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — two one-line re.matchre.fullmatch tightenings mirroring the fix already merged to main (#3076), each with a regression test. This also addresses my earlier comment: the ResourceTemplate.matches() trailing-newline instance flagged there is now fixed in this PR. The one inline note below is a pre-existing looseness in the same function and doesn't block this backport.

Extended reasoning...

Overview

Backport of #3076 to the v1.x branch. Two production changes, each a single line: tool_name_validation.py switches the SEP-986 tool-name check from TOOL_NAME_REGEX.match(name) to .fullmatch(name), and ResourceTemplate.matches() switches re.match(f\"^{pattern}$\", uri) to re.fullmatch(pattern, uri). Both close the classic Python pitfall where $ also matches just before a single trailing newline. Regression tests are added for both (trailing-newline tool names including the 128-char boundary case, and a trailing newline after a literal template segment).

Security risks

The change is strictly tightening — every input rejected before is still rejected, and inputs with a trailing newline that previously slipped through are now rejected. It closes a minor input-validation gap (newline smuggled past validation into tool names / template matches). No new attack surface is introduced. The remaining looseness (newlines inside parameter segments via [^/]+, and unescaped regex metacharacters in template literals) is pre-existing, also present on main, explicitly acknowledged in the PR description, and covered by the inline comment.

Level of scrutiny

Low. The semantics of re.fullmatch(p, s) vs re.match(f\"^{p}$\", s) differ only in the trailing-newline case, so the behavioral delta is exactly the intended fix. The change mirrors a fix already reviewed and merged to main, and the fix direction (\\A...\\Z / fullmatch) is already established elsewhere in the codebase (task_scope.py).

Other factors

My previous review comment on this PR asked for exactly the templates.py half of this change, and the PR was updated to include it — prior feedback is fully addressed. The bug hunting system's only finding this run is the pre-existing regex-escaping issue in template literals, posted inline as a follow-up suggestion, not a blocker. A candidate issue about parameter segments still accepting embedded newlines after the fullmatch fix was examined and ruled out as a distinct pre-existing looseness deliberately left unchanged (and acknowledged in the PR description).

Comment on lines 88 to +89
pattern = self.uri_template.replace("{", "(?P<").replace("}", ">[^/]+)")
match = re.match(f"^{pattern}$", uri)
match = re.fullmatch(pattern, uri)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing issue, not introduced by this PR: the template-to-regex conversion on line 88 never re.escape()s the literal (non-parameter) segments, so regex metacharacters in a template are interpreted as regex syntax — e.g. template test://a.b/{x} matches test://aXb/foo because . is a wildcard, and literals containing ( raise re.error at match time. Since this PR already tightens this exact line via re.fullmatch, a natural follow-up is to split the template on {param} boundaries and re.escape() the literal chunks before joining them with the named-group patterns.

Extended reasoning...

The bug

ResourceTemplate.matches() builds its regex by naive string replacement:

pattern = self.uri_template.replace("{", "(?P<").replace("}", ">[^/]+)")
match = re.fullmatch(pattern, uri)

Only the {/} delimiters are transformed — the literal (non-parameter) segments of the URI template are spliced into the regex verbatim, with no re.escape(). Any regex metacharacter in a literal segment (., +, ?, *, (, ), [, |, ...) is therefore interpreted as regex syntax instead of being matched literally.

Step-by-step proof

Take the template test://a.b/{x}:

  1. The .replace() calls produce the pattern test://a.b/(?P<x>[^/]+).
  2. The . in a.b is an unescaped regex wildcard, so it matches any character, not just a literal dot.
  3. re.fullmatch('test://a.b/(?P<x>[^/]+)', 'test://aXb/foo') succeeds — . consumes X.
  4. matches() returns {'x': 'foo'}, and the URI is dispatched to a handler whose template it does not actually match.

Two worse variants were verified empirically:

  • Semantics change: template test://a+b/{x} compiles a+ as "one or more a", so it matches test://ab/foo and test://aab/foo — and, depending on the surrounding literal, can fail to match its own literal form test://a+b/foo.
  • Crash: a template containing a bare ( in a literal segment (e.g. test://a(b/{x}) raises re.error: missing ), unterminated subpattern at match time, turning a registration-time template typo into a runtime exception on every read_resource lookup.

Why nothing prevents it

There is no escaping or validation anywhere on this path: ResourceTemplate.from_function stores uri_template as a plain string, and matches() is the only place it is interpreted. The wire path's pydantic AnyUrl normalization does not help here — it normalizes the incoming URI, not the server-side template, and dots/plus signs are perfectly legal URL characters that survive normalization.

Relation to this PR

This is squarely pre-existing: the naive .replace() construction on line 88 is untouched by this PR, which only switched the adjacent re.match(f"^{pattern}$", uri) to re.fullmatch(pattern, uri) — a strictly tighter change that neither introduces nor worsens the escaping gap. It's worth noting only because the PR modifies this exact function and fixes the same bug class (overly-permissive template matching), so a follow-up here would be a natural companion.

How to fix

Split the template on {param} boundaries and escape the literal chunks before reassembling, e.g.:

parts = re.split(r"\{(\w+)\}", self.uri_template)
pattern = "".join(
    f"(?P<{part}>[^/]+)" if i % 2 else re.escape(part)
    for i, part in enumerate(parts)
)
match = re.fullmatch(pattern, uri)

This keeps the existing [^/]+ parameter semantics while making literal segments match literally, and eliminates the re.error crash for templates containing regex metacharacters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant